-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Add .diff and .patch support to compare #34433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Some things in my mind:
|
3acbb9f
to
bc79694
Compare
d67ae3a
to
afae382
Compare
afae382
to
c80f0b9
Compare
@wxiaoguang |
routers/web/repo/compare.go
Outdated
@@ -265,7 +263,8 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { | |||
} | |||
return nil | |||
} | |||
ci.HeadBranch = headInfos[1] | |||
// FIXME: how to correctly choose the head repository? The logic below (3-8) is quite complex, the real head repo is determined there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When trying to understand the logic, I find something strange, so I left a FIXME here.
Do you have ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ps: if it is too complex and we don't really need to support these cases, we can also skip the diff/patch support for these cases and leave some comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll address that, I prefer to support all use cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wxiaoguang is it clearer now?
I think i covered all use cases with integration tests as well, you think there is any other case we need to cover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so.
- The new logic is inconsistent with the steps 3-8 below. The "headRepo" guess is quite complex.
- I do not think GetRepoRawDiffForFile would work for different base&head repositories, for example:
ref1...headRepo:ref2
, but what if ref1 only exists in base repo and ref2 only exists in head repo?
Implements proposal #34410

